Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[project-library-manage] ライブラリのインストール・アンインストール処理を実装 #1497

Open
wants to merge 76 commits into
base: project-library-manage
Choose a base branch
from

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Aug 10, 2023

内容

題のとおりです。
ライブラリインストール時は、ライブラリの利用規約を一度目に通すように経路を組みました。
ライブラリのダウンロードはフロントエンド側で行い、メモリ上のみに記憶しておくほうが楽そうに見えたのですが、ライブラリというものが基本大きいファイルであるため、一度一時ファイルとして保存するようにしました。
本当はcallbackを使ってインストール完了後にいい感じの処理ができればいいのですが、ipcしか使えない関係上少しごちゃっとしています。

関連 Issue

スクリーンショット・動画など

image image

その他

動作確認に、音声ライブラリ管理機能を使えるようにしたSHAREVOXエンジンを使っていましたが、インストール機能周辺にバグがあったので0.3.0-preview.2 0.3.0-preview.3 0.3.0-preview.4を作りました。以下からダウンロードして使えます。
https://github.com/SHAREVOX/sharevox_engine/releases/tag/0.3.0-preview.2
https://github.com/SHAREVOX/sharevox_engine/releases/tag/0.3.0-preview.3
https://github.com/SHAREVOX/sharevox_engine/releases/tag/0.3.0-preview.4

@y-chan y-chan requested a review from a team as a code owner August 10, 2023 15:42
@y-chan y-chan requested review from Hiroshiba and removed request for a team August 10, 2023 15:42
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと細かいところはまだ見られてないのですが、一旦設計レベルの話をちょこちょこ書いてみました 🙇

src/background/libraryManager.ts Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/store/engine.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/background/libraryManager.ts Outdated Show resolved Hide resolved
src/electron/preload.ts Outdated Show resolved Hide resolved
Copy link
Member Author

@y-chan y-chan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

結構時間かかってしまいましたが、レビューいただいた点を改善しました!
最初に比べて、かなりきれいな実装に持ってこれたんじゃないかなと思います...!
特にいじってない点や意見のある部分に関してはいじらずにそのままにして、コメントつけておきました(2箇所ほど)

src/components/LibraryInstallDialog.vue Outdated Show resolved Hide resolved
src/store/library.ts Show resolved Hide resolved
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

めちゃめちゃ良いコードになりましたね!!!
「バックグラウンド側でダイアログが出るからこっちではダイアログを出す必要がない」みたいなコメントもすごい優しいなと思いました。真似していきたいです。

もう1回ソースコード全部見直したいなと思っていますが、ちょっと時間がかかりそうだったので変更箇所周りだけコメントさせていただきました 🙇

package.json Show resolved Hide resolved
src/background/libraryManager.ts Outdated Show resolved Hide resolved
src/background/libraryManager.ts Outdated Show resolved Hide resolved
src/background/libraryManager.ts Outdated Show resolved Hide resolved
src/background/libraryManager.ts Outdated Show resolved Hide resolved
src/background/libraryManager.ts Outdated Show resolved Hide resolved
src/components/LibraryInstallDialog.vue Show resolved Hide resolved
src/components/LibraryInstallDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryInstallDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryInstallDialog.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@thiramisu thiramisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと内部処理までは追えなさそうなのですが、主にUIに関して数か所コメントさせていただきました。

src/store/library.ts Outdated Show resolved Hide resolved
src/browser/sandbox.ts Outdated Show resolved Hide resolved
</div>
<div class="q-px-md q-pt-md">
<span class="text-h5 text-bold">
各話者・キャラクターの利用規約をお読みください。
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"話者"という表現がエディタのユーザー向けに表示されるのは現状1箇所しかないので、単に「各キャラクター」の方が分かりやすい気がしますが、どうでしょう?
(もしstyleId毎を意図しているなら「各キャラクター・スタイル」ですかね)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nemo音声や他のエンジンのことも考えると確かに「話者」という表現の検討も必要かもと思いました!
でもとりあえずキャラクターで統一しておいて、後でどうするか決めるのが良さそう・・・・・・?

Copy link
Member Author

@y-chan y-chan Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nemo音声や他のエンジンのことも考えると

私が意図していたのはここでした、他エンジンの場合必ずしもキャラクターではない場合があるかなと思ったので。
まあでもVOICEVOX UIに置いてはあらゆるところで「キャラクター」表記が使われているので、一旦統一してIssue化して議論するのがいいのかなと思いました。

src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue Outdated Show resolved Hide resolved
@y-chan
Copy link
Member Author

y-chan commented Sep 19, 2023

project-library-manageブランチをマージし、動作確認ができるようになりました...!
SHAREVOX Engineを用いて動作確認する場合はSHAREVOX Engine 0.3.0-preview.4をインストールしてからであれば問題なく動作確認できると思います...!

P.S. e2eテストを書きたいなと思ったのですが、Nemoエンジンを使ってる関係上難しいのかなと思いました...どうするのがいいんですかね...

@Hiroshiba
Copy link
Member

@y-chan

P.S. e2eテストを書きたいなと思ったのですが、Nemoエンジンを使ってる関係上難しいのかなと思いました...どうするのがいいんですかね...

これめっちゃ難しいですね。。。
とりあえずNemoエンジンのmasterブランチを追従させて、0.15.0-preview.0を作れば良さそう・・・・・?

あるいはVOICEOX OSS的には、キャラクターが何もインストールされていないプレーンなVOICEVOXエンジンをビルドできると最高にかっこいいかなと思いました。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants